Use StringData for Nix language value documentation too
#14527
Conversation
github-actions
bot
added
new-cli
|
🎉 All dependencies have been resolved ! |
| .args = {}, | ||
| .arity = (size_t) arity, | ||
| .doc = doc, | ||
| .doc = &nix::StringData::make(doc), |
There was a problem hiding this comment.
This seems to be a reference to a local temporary StringData which promptly becomes a dangling pointer.
There was a problem hiding this comment.
It is turning a reference into a pointer, and the reference is to a freshly heap-allocated thing, so actually it is fine.
(It errs on the side of leaking without GC, not on the side of the lifetime being too short and getting heap corruption.)
| * Optional free-form documentation about the primop. | ||
| */ | ||
| const char * doc = nullptr; | ||
| const StringData * doc = nullptr; |
There was a problem hiding this comment.
PrimOp is usually not traced, so these StringData would get collected and become a dangling reference.
It could be done by allocating the native primops on the heap or with traceable_allocator.
The C API does allocate them on the GC heap because I needed dynamically allocated ones for NixOps4, but the other, "static" primops will reference garbage.
There was a problem hiding this comment.
I think I made these all be statically allocated StringData with the _sds thing.
| * Optional free-form documentation about the constant. | ||
| */ | ||
| const char * doc = nullptr; | ||
| const StringData * doc = nullptr; |
| * `null`. | ||
| */ | ||
| const char * doc; | ||
| const StringData & doc; |
There was a problem hiding this comment.
This should probably be similar to the RootValue solution so that Doc can be manipulated as an idiomatic C++ object.
Ericson2314
force-pushed
the
string-data-doc
branch
from
755e3a8 to
110e1d0
Compare
Trailing comma helps a lot.
This probably isn't much of a perf gain, but it does mean we are completely rid of non-length prefixed strings. That allows us to delete some code.
Now that it isn't just used for `Value`, it doesn't really belong in there. Rename `static-string-data.hh` to share the same prefix to keep them close together when sorting lines, also.
Ericson2314
force-pushed
the
string-data-doc
branch
from
110e1d0 to
ddca102
Compare
2 participants
Motivation
This probably isn't much of a perf gain, but it does mean we are completely rid of non-length prefixed strings. That allows us to delete some code.
Context
Depends on #14442
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.